Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes references to slugs throughout the Tower CLI codebase as part of a platform-wide effort to eliminate slug-based identifiers. The changes transition from slug-based to name-based identifiers for apps, catalogs, and related resources while maintaining backward compatibility for legacy CLI clients.
Key changes:
- Replace slug parameters with name parameters in API endpoints and CLI commands
- Convert slug fields to optional deprecated fields in model classes
- Update CLI commands to use app names instead of slugs
- Remove slug requirement from app creation workflow
Reviewed Changes
Copilot reviewed 183 out of 183 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/tower/tower_api_client/models/*.py |
Updated model classes to make slug fields optional and deprecated, replaced with name-based fields |
src/tower/tower_api_client/api/default/*.py |
Changed API endpoint parameters from slug to name across all app and catalog operations |
crates/tower-cmd/src/apps.rs |
Removed slug argument from app creation command and updated references to use names |
crates/tower-cmd/src/api.rs |
Updated API client calls to use name parameters instead of slug |
crates/tower-api/src/models/*.rs |
Updated Rust model definitions to reflect API changes with name-based fields |
|
|
||
| parameters = [] | ||
| _parameters = d.pop("parameters", UNSET) | ||
| for parameters_item_data in _parameters or []: |
There was a problem hiding this comment.
Potential bug: The parameters list is initialized as empty but then populated in a loop. If _parameters is UNSET, the loop will iterate over an empty list, which is correct. However, if _parameters is None, the loop will fail. The code should handle the case where _parameters might be None.
| for parameters_item_data in _parameters or []: | |
| if _parameters is None or isinstance(_parameters, Unset): | |
| _parameters = [] | |
| for parameters_item_data in _parameters: |
There was a problem hiding this comment.
Generated code, ignoring this feedback.
| parameters = [] | ||
| _parameters = d.pop("parameters", UNSET) | ||
| for parameters_item_data in _parameters or []: | ||
| parameters_item = RunParameter.from_dict(parameters_item_data) | ||
|
|
||
| parameters.append(parameters_item) | ||
|
|
There was a problem hiding this comment.
Potential bug: Similar to update_schedule_params.py, the parameters list initialization and population pattern could fail if _parameters is None rather than UNSET. The code assumes _parameters or [] will handle None values, but this should be made more explicit.
| parameters = [] | |
| _parameters = d.pop("parameters", UNSET) | |
| for parameters_item_data in _parameters or []: | |
| parameters_item = RunParameter.from_dict(parameters_item_data) | |
| parameters.append(parameters_item) | |
| _parameters = d.pop("parameters", UNSET) | |
| if _parameters is None or _parameters is UNSET: | |
| parameters = [] | |
| else: | |
| parameters = [] | |
| for parameters_item_data in _parameters: | |
| parameters_item = RunParameter.from_dict(parameters_item_data) | |
| parameters.append(parameters_item) |
There was a problem hiding this comment.
Generated code, ignoring this feedback.
| name: app_slug.to_string(), | ||
| slug: Some(app_slug.to_string()), | ||
| name: app_name.to_string(), | ||
| short_description: Some(description.to_string()), |
There was a problem hiding this comment.
[nitpick] The slug field is being set to None here, but the comment and context suggest this is for backward compatibility. Consider adding a comment explaining why slug is set to None and its relationship to the legacy CLI support.
| short_description: Some(description.to_string()), | |
| short_description: Some(description.to_string()), | |
| // The `slug` field is set to `None` for backward compatibility with legacy CLI support. |
There was a problem hiding this comment.
Not useful. Ignoring.
| *DefaultApi* | [**list_alerts**](docs/DefaultApi.md#list_alerts) | **GET** /alerts | List alerts | ||
| *DefaultApi* | [**list_api_keys**](docs/DefaultApi.md#list_api_keys) | **GET** /api-keys | List API keys | ||
| *DefaultApi* | [**list_app_environments**](docs/DefaultApi.md#list_app_environments) | **GET** /apps/{slug}/environments | List app environments | ||
| *DefaultApi* | [**list_app_environments**](docs/DefaultApi.md#list_app_environments) | **GET** /apps/{name}/environments | List app environments |
There was a problem hiding this comment.
i mean yeah sure but the PR isn't merged in yet 😅
There was a problem hiding this comment.
This is a different API endpoint, already exists! We will probably want to deprecate it when we're done with your environments work: https://docs.tower.dev/docs/reference/api/list-app-environments
socksy
left a comment
socksy
left a comment
There was a problem hiding this comment.
The schedules stuff is autogenerated I assume — after we do some of the schedules API changes Giray requested we're gonna have to re autogenerate.
bradhe
deleted the
feature/tow-586-update-cli-to-remove-references-to-slugs-everywhere
branch
Since we're purging slugs from the platform entirely, we need to update the CLI to work with this new scheme.